Send origin-form request-targets - #44
Conversation
![carpentry-heartbeat[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
RFC 9112 §3.2.1 requires a client talking directly to an origin server to
send only the absolute path and query as the request-target; absolute-form
(§3.2.2) addresses a proxy. `target` re-rendered the whole URI, so a request
built from an absolute URL went out as
GET http://127.0.0.1:8843/good HTTP/1.1
Host: 127.0.0.1
Conforming servers accept that, which is why nothing broke. What it costs is
the ambiguity §3.2.2 spends a paragraph on — a request line carrying a scheme
and host that the Host header also carries — and stricter intermediaries treat
absolute-form from a non-proxy client as suspicious.
Compose the target from URI.path and URI.query rather than re-rendering the
URI. Dropping scheme, host, port, userinfo and fragment now falls out of
reading only two components instead of needing them cleared first. URI.full-path
does the same job, but the pinned uri@0.5.1 prepends `/` unconditionally, so a
path set to "/a/b" by hand comes back "//a/b"; the leading-slash guard here is
what uri itself grew after 0.5.1.
The suite pinned the old shape ("absolute-form request-target is left alone");
those assertions now pin origin-form. New cases cover an already absolute path,
a URI carrying path, query, userinfo, port and fragment at once, and that host
and port survive on the request's URI so http-client keeps building its Host
header.
The other three §3.2 forms stay unproduced: nothing here or in http-client
speaks to a proxy or opens a tunnel, and a bare "host:port" parses as
scheme+opaque rather than host+port, so authority-form would need an API with
no caller.
Closes #43
![carpentry-reviewer[bot]](https://avatars.githubusercontent.com/u/44377836?s=60&v=4){kind=small}
There was a problem hiding this comment.
Build & Tests
carp -x test/http.carp at 84bb203 on this armhf Pi — 491 passed, 0
failed, rc read from the unpiped command. CI green on both legs. carp -x gendocs.carp exits 0 and leaves the tree clean, and docs/index.html really is
byte-identical to docs/http_index.html. This repo's CI gates on angler and
carp-fmt with no continue-on-error anywhere in the workflow, so those two
claims are carried by the green run rather than by my local binaries.
The new target is pinned on every branch it has. Four mutants, suite
re-run against each:
| mutant | result |
|---|---|
always prepend / |
490/1 — killed |
never prepend / |
475/16 — killed |
always append ? |
480/11 — killed |
| drop the query | 486/5 — killed |
The first is the one worth naming: it is killed by exactly one assertion,
an already absolute path is not slashed twice, which this PR adds. Without it
the leading-slash guard — the whole reason URI.full-path was not used — would
have shipped unpinned.
Test accounting checks out, which is worth saying given the last three rounds:
9 expectations spelling out GET http://… change (1 inverted + the 8 the
body claims), 3 assertions are genuinely new, 0 removed.
I also ran 20 URIs through target on master and on this branch. Everything
the body describes reproduces: http://example.com → /, the userinfo/fragment
strips, CONNECT foo.com:443 → /, mailto: → /, and * → /* on both
(so asterisk-form really was unreachable before, as you say).
Findings
1. http-client does not set Host the way this PR says it does — and that is the one claim the follow-up rests on
it already sets
HostfromURI.host(http-client.carp:376-378), which is
what origin-form needs
It sets Host from URI.host and nothing else — http-client.carp:370-379
binds host to (Maybe.from @(URI.host &uri) @"") and puts that value straight
into the header. The port is bound separately, used for connect, and never
reaches the request. RFC 9110 §7.2 requires the field value to be the whole
authority, port included.
That is http-client's pre-existing bug, not this diff's — but on master the
port was still on the wire, in the request line. Rendering the same request both
ways:
master: GET http://127.0.0.1:8843/a/b?q=1 HTTP/1.1
Host: 127.0.0.1
branch: GET /a/b?q=1 HTTP/1.1
Host: 127.0.0.1
After the pin bump this PR recommends, port 8843 appears nowhere in the
request. Feeding both captures to a real origin server and reconstructing the
absolute URL the way every framework does for redirects and Location headers:
master: http://127.0.0.1http://127.0.0.1:8843/a/b?q=1 (garbage, obviously broken)
branch: http://127.0.0.1/a/b?q=1 (plausible, silently wrong port)
Both are wrong; the second is the worse kind. And http-client's suite cannot
catch it — you already note it routes on a path suffix, and its test server
never reads Host.
So: nothing to change here, but the sentence should not say the invariant holds,
because the next person to read it will bump the pin. The honest form is that
origin-form makes Host load-bearing and http-client is not yet ready for it.
Worth an issue on http-client alongside #23 so the bump has a prerequisite.
2. Recorded, not asked for: an empty-but-present query is dropped
http://h.example/a? renders http://h.example/a? on master and /a here;
same for a hand-built (URI.set-query (URI.zero) (Maybe.Just @"")), /? before
and / now. The ? guard tests String.empty? on the extracted value, so it
cannot tell an empty query from an absent one.
I am not asking for a change. I went looking for a downstream that cares and
did not find one — Python's urlsplit(...).geturl() normalises http://h.example/a?
to http://h.example/a too. Flagging it only because the "Pinned behaviour that
changes" section enumerates every other shift down to mailto:, and this one is
not in it.
Not a finding, checked because it looked like one
http://h.example//a/b renders /a/b — one slash short. master renders
http://h.example/a/b, equally short, so this is uri@0.5.1 dropping a leading
slash on parse and not a regression here. Mentioning it so the next reader does
not re-derive it.
Verdict: merge
Does exactly what you asked for in #43, the request line is right per §3.2.1,
the three unproduced forms are disclosed rather than glossed, and the tests hold
every branch of the new code. Finding 1 is a wrong sentence about another repo
rather than a defect in this diff — but it is the sentence that would license
the http-client pin bump, and that bump would silently drop the port from
every non-default-port request.
1 participant
Fixes #43, the way you called it in the issue thread:
Request.strnow renders the RFC-intended target.What changed
Request.targetre-rendered the whole URI, so a request built from an absolute URL went on the wire in absolute-form — the form RFC 9112 §3.2.2 reserves for a request to a proxy:It now renders origin-form, which §3.2.1 requires of a client talking directly to an origin server:
The implementation composes the target from
URI.pathandURI.queryinstead of re-rendering the URI and subtracting from it. Dropping scheme, host, port, userinfo and fragment now falls out of reading only two components, so the explicitset-user/set-password/set-fragmentclearing is gone.Why not just call
URI.full-pathThat is exactly the right function and it is what a future pin should use — but the pinned
uri@0.5.1prepends/unconditionally:0.5.1 parses paths without their leading slash (its own
URI.strre-adds one via aslash?guard), sofull-pathis right for every parsed URI and wrong for a hand-set absolute one.urigrew the same leading-slash guard after 0.5.1, infc8c76b. Rather than ship a known double-slash for hand-built URIs, the guard lives here; it can collapse to afull-pathcall whenever the pin moves past 0.5.1.Pinned behaviour that changes
test/http.carppinned the old shape —"absolute-form request-target is left alone"— and that assertion is now inverted: the same URI rendersGET / HTTP/1.1. Eight further assertions that spelled outGET http://h.example/a/c …change toGET /a/c …. They still pin what they were written to pin (userinfo stripped, fragment stripped, encoded#kept in the query); only the expected request line moved.New cases cover the edges:
http://example.com(scheme + host, no path)/http://user:pw@h.example:8080/a/b?q=1#sec2/a/b?q=1/a/bby hand/a/b, not//a/bURI.zero, query-only, userinfo-only, fragment-onlyplus one asserting the host and port are still on
Request.uriafter rendering — that is the invarianthttp-clientleans on to build itsHostheader, and origin-form makes it load-bearing rather than belt-and-braces.Effect on
http-clienthttp-clientpinshttp@0.4.2and only callsRequest.str, somasteris untouched by this. Bumping the pin would flip every request it sends to origin-form with no code change on its side; it already setsHostfromURI.host(http-client.carp:376-378), which is what origin-form needs. Its suite routes on a path suffix, so it should stay green — worth re-running at bump time rather than taken on faith.The other three §3.2 forms — out of scope
Deliberately not implemented, and I want to be explicit about the one that regresses:
http-client, so there is nothing to select it with. A flag or second renderer is easy to add when a proxy actually exists to point at.CONNECT) — this one changes.(Request.request @"CONNECT" (URI.parse "foo.com:443") …)used to renderCONNECT foo.com:443 HTTP/1.1, which is correct — but only by accident:URI.parsereadsfoo.com:443as schemefoo.com+ opaque443, and the oldtargetechoedURI.strback. It now rendersCONNECT /. Supporting it properly means renderinghost:portfrom a URI that has a real host and port (https://foo.com:443), which is a different input shape and a mapping with no caller in the org — speculative until something opens a tunnel.OPTIONS) — unchanged, and already unsupported before this:URI.parse "*"yields path*, whoseURI.stris/*, soOPTIONS * HTTP/1.1was not reachable onmastereither. A URI is the wrong carrier for*; expressing it needs an API change.One more shift worth naming: an opaque URI such as
mailto:a@b.comused to render itself and now renders/, since it has neither path nor query. Sending amailto:target over HTTP is meaningless in either form.Request.str's docstring now states the wire format and names the three unproduced forms;docs/Request.htmlis regenerated anddocs/index.htmlstill matchesdocs/http_index.htmlbyte-for-byte. No CHANGELOG — this repo does not keep one.Checks
carp -x test/http.carp— 491 passed, 0 failedcarp-fmt --checkandanglerover the CI file set — clean, withanglerbuilt from its currentHEADso the newbyte-offset-as-char-indexrule was in playcarp -x gendocs.carp— cleanOpened by the carpentry-org heartbeat agent (Claude). Veit has not reviewed this yet.